@addaleax build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1323/pipeline

CI: https://ci.nodejs.org/job/node-test-pull-request/18022/

@nodejs/tsc PTAL … if there are objections standing on Wednesday I’ll call for a vote.

I don’t think this is a good one. I would prefer a PR that throwed a JS error instead of crashing. Why is that approach unfeasible?

@mcollina I was going for the existing behavior on v10.9.0 and below to avoid a breaking change… I’d be happy to implement that as a semver-major follow up, or, if the TSC thinks it could land on v10.x and v11.x, we could make an exception and add typechecking in a patch release?

Edit: I’ve also decided to not go for the breaking change because the fact that there’s an issue on this demonstrates that this does affect in-the-wild code. I fully agree with throwing, but I’d prefer to keep breaking changes semver-major…

+1 as a follow-up semver-major.

(Oops, multiple reviews submitted during the GitHub outage)

Edit: I’ve also decided to not go for the breaking change because the fact that there’s an issue on this demonstrates that this does affect in-the-wild code. I fully agree with throwing, but I’d prefer to keep breaking changes semver-major…

BTW the bug (#23668) was opened for v11.0.0-nightly20181007061e09891c.

The actual bug (wyhaya/updns#7) is in https://github.com/wyhaya/updns/blob/4fb62c6fa6863089c8443d737ad1de61031280af/lib/parse.js#L125-L126

    question.qtype.copy(buf, 12 + qname.length, question.qtype, 2)
    question.qclass.copy(buf, 12 + qname.length + 2, question.qclass, 2)

third argument is the buffer, and not a bufferStart number

This module has been failing silently since the module was published, possibly leaking memory content.

So IIUC we have a crash instead of a silent fail in the node10 line, and that is going like that into LTS?
Also node11 just went out with this behaviour.
So IMHO there's an argument to be made that turning a crash into an exception is semver-patch.

This returns back to the previous behavior of defaulting to 0,
which is certainly questionable.

P.S. The previous behavior is undefined, it's not default to 0.
e.g. on node 10.9.0

> const b = Buffer.allocUnsafe(1024);
> const c = Buffer.allocUnsafe(512);
> b.copy(c, 'not a valid offset');
512
> b.copy(c, '4');
508
> const d = Buffer.alloc(1)
> d[0]='8'.charCodeAt(0)
> b.copy(c, d);
504

So this PR is semver-major

Summery:

buf.copy(target[, targetStart[, sourceStart[, sourceEnd]]])

Added in: v0.1.90

• target | A Buffer or Uint8Array to copy into.
• targetStart The offset within target at which to begin writing. Default: 0.
• sourceStart The offset within buf from which to begin copying. Default: 0.
• sourceEnd The offset within buf at which to stop copying (not inclusive). Default: buf.length.
• Returns: The number of bytes copied.

node <= 10.9.0:

> const b = Buffer.allocUnsafe(1024);
undefined
> const c = Buffer.allocUnsafe(512);
undefined
> b.copy(c, 8)
504
> b.copy(c, 8, 'gaga')
504
> b.copy(c, '8', '600')
424
> b.copy(c, 'gaga', 600)
424
> b.copy(c, '112', 600)
400

node >= 10.10.0 || 11.0.0

> const b = Buffer.allocUnsafe(1024);
undefined
> const c = Buffer.allocUnsafe(512);
undefined
> b.copy(c, 8)
504
> b.copy(c, 8, 'gaga')
# node11.0.0.exe  [27956]: src\node_buffer.cc:173: Assertion `arg->IsNumber()' failed.

i.e. Crash

this patch

> const b = Buffer.allocUnsafe(1024);
undefined
> const c = Buffer.allocUnsafe(512);
undefined
> b.copy(c, 8)
504
> b.copy(c, 'gaga')
512
> b.copy(c, '8')
512
> b.copy(c, 'gaga', 600)
424
> b.copy(c, '112', 600)
424

my suggestion

> const b = Buffer.allocUnsafe(1024);
undefined
> const c = Buffer.allocUnsafe(512);
undefined
> b.copy(c, 8)
504
> b.copy(c, 'gaga')
RangeError: Index out of range
    at Buffer.copy (buffer.js:637:12)
> b.copy(c, '8')
RangeError: Index out of range
    at Buffer.copy (buffer.js:637:12)
> b.copy(c, 'gaga', 600)
RangeError: Index out of range
    at Buffer.copy (buffer.js:637:12)
> b.copy(c, '112', 600)
RangeError: Index out of range
    at Buffer.copy (buffer.js:637:12)

After talking a bunch with @refack and doing a minimal bit of tinkering, it does seem like this does not quite restore the previous behavior. This might duplicate comments above by @refack, but this is a small behavior change:

Node.js 10.9.0 (which is the version with the behavior we wish to restore):

> var b = Buffer.allocUnsafe(1024)
undefined
> var c = Buffer.allocUnsafe(512)
undefined
> b.copy(c, '112', 600)
400
> b.copy(c, 112, 600)
400
>

With this patch:

$ ./node
> var b = Buffer.allocUnsafe(1024)
undefined
> var c = Buffer.allocUnsafe(512)
undefined
> b.copy(c, '112', 600)
424
> b.copy(c, 112, 600)
400
> 

Can we patch that up? Or is that not desirable for a reason I don't see?

I’ve re-targeted this PR to v10.x, under the assumption that we can do something like #23840 a) for all affected methods and b) without significant performance impact.

CI: https://ci.nodejs.org/job/node-test-pull-request/18150/

@nodejs/tsc ... We have two competing PRs addressing the same issue. We need a resolution. This PR is one option, #23875 is the other.

I personally defer to the review of folks that know this code but would like to see something land ASAP so we can test 10.13.0 before pushing it out

@MylesBorins for the record (as the person who originally made the change that broke things, sorry for that) I'd prefer this one over #23875 as long as it uses V8 functions that'll be deprecated soon.

With regards to v10.x:

https://github.com/nodejs/node/blob/f14274714d00bb5b58482bef15e84cdda9237910/COLLABORATOR_GUIDE.md#when-breaking-changes-actually-break-things

• If a breaking commit does accidentally land in a Current or LTS branch, an attempt to fix the issue will be made before the next release; If no fix is provided then the commit will be reverted.

2555cb4 is the breaking commit that landed in v10.x (v10.10.0, although the changelog has this as c637d41b9d).

#23795 is an attempt to fix the issue while #23875 is a revert.

#23795 has a number of approvals, so IMHO can land as a fix.

The alternative is to revert (#23875) and then immediately re-backport #22129 including the changes from #23795.

@MylesBorins @richardlau I think all we need at this point is a green CI.

We could either rebase out the commit that broke the bootstrap module count for Workers CI, or land #23876 as soon as we can.

Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/18153/

Another attempt at CI: https://ci.nodejs.org/job/node-test-pull-request/18166/

Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/18176/

I’ve removed #23625 from v10.x-staging.

CI should be good now: https://ci.nodejs.org/job/node-test-pull-request/18181/

Resume CI again: https://ci.nodejs.org/job/node-test-pull-request/18186/

Another CI attempt: https://ci.nodejs.org/job/node-test-pull-request/18193/

Sigh, resume CI again: https://ci.nodejs.org/job/node-test-pull-request/18208/
Again: https://ci.nodejs.org/job/node-test-pull-request/18209/
Again: https://ci.nodejs.org/job/node-test-pull-request/18210/
Again: https://ci.nodejs.org/job/node-test-pull-request/18211/

I've reimplemented the workaround for #22006
https://ci.nodejs.org/job/node-test-pull-request/18213/
The better solution is to backport #23733

landed in the v10.13.0-proposal branch in 2ba6010